New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce ApplicationRecord, an Active Record layer supertype #22567
Introduce ApplicationRecord, an Active Record layer supertype #22567
Conversation
r? @chancancode (@rails-bot has picked a reviewer for you, use r? to override) |
2ed43e3
to
851deb6
Compare
Do we need to test the case in which a user creates a model called It might not be too far-fetched as an option. Maybe 'ApplicationRecord' can be included in the words that cannot be used as a name for a model. Or a warning could be displayed if such a model exists. |
That may be an idea. Maybe we can warn on some "reserved" names in all the class ApplicationController < ApplicationController
end |
This is way too defensive to me. I don't think anyone would have a model On Sat, Dec 12, 2015, 15:26 Genadi Samokovarov notifications@github.com
|
in that case, no objections 🎀 |
851deb6
to
5059dd6
Compare
It's pretty common for folks to monkey patch `ActiveRecord::Base` to | ||
work around an issue or introduce extra functionality. Instead of | ||
shoving even more stuff in `ActiveRecord::Base`, `ApplicationRecord` can | ||
hold all those custom work the apps may need. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should rephrase the motivation. What I've understood to be the issue is that engines will force configuration changes onto an application, and that there's no way to keep them separate because both were configuring ActiveRecord::Base
. That, in turn, results in monkey patches.
You are right about the convenience of where to include extra app wide record functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! We can and should advertise that to plugin authors. I have AR extensions myself, which I can distribute as modules to be mixed into ApplicationRecord
instead of ActiveRecord::Base
monkey patches.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes we need to rephrase the motivation to talk about engines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My point wasn't whether or not we should advertise to engine authors. It was that the motivation stated here isn't why we want to add this class.
As part of this commit, you might also want to change these lines of the README so people learn not to inherit directly from
|
I am also wondering whether we should change tests/guides/docs to refrain from referencing |
c533f43
to
84da9c0
Compare
Yo! I tried to touch a bit on the documentation, I'm sure I haven't replaced all the references, but at least the @kaspth can you check the changelog entry again? |
ActiveRecord::Base.include(Yaffle::ActsAsYaffle) | ||
# test/dummy/app/models/application_record.rb | ||
|
||
class ApplicationRecord < ActiveRecord::Base |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to sell this, so I can hint the plugin folks. Tell me if you think it's an overkill.
ce76f20
to
762e4df
Compare
It's pretty common for folks to monkey patch `ActiveRecord::Base` to work around an issue or introduce extra functionality. Instead of shoving even more stuff in `ActiveRecord::Base`, `ApplicationRecord` can hold all those custom work the apps may need. Now, we don't wanna encourage all of the application models to inherit from `ActiveRecord::Base`, but we can encourage all the models that do, to inherit from `ApplicationRecord`. Newly generated applications have `app/models/application_record.rb` present by default. The model generators are smart enough to recognize that newly generated models have to inherit from `ApplicationRecord`, but only if it's present.
762e4df
to
2067fff
Compare
@@ -1,3 +1,18 @@ | |||
* Introduce ApplicationRecord, an Active Record layer super type. | |||
|
|||
An `ApplicationRecord` let's engines have models, isolated from the main |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you folks pass over the motivation again?
Introduce ApplicationRecord, an Active Record layer supertype
❤️ 💚 💙 💛 💜 |
Thanks for the help, guys! I'll try to follow up with more documentation changes. |
end | ||
|
||
def determine_default_parent_class | ||
if File.exist?('app/models/application_record.rb') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it safe to expect that the app root is the current working directory here, or should it check for the file presence relative to application root?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll check if thor has a helper for that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made the check to be relative to the application root here 342221b
It's pretty common for folks to monkey patch
ActiveRecord::Base
towork around an issue or introduce extra functionality. Instead of
shoving even more stuff in
ActiveRecord::Base
,ApplicationRecord
canhold all those custom work the apps may need.
Now, we don't wanna encourage all of the application models to inherit
from
ActiveRecord::Base
, but we can encourage all the models that do,to inherit from
ApplicationRecord
.Newly generated applications have
app/models/application_record.rb
present by default. The model generators are smart enough to recognize
that newly generated models have to inherit from
ApplicationRecord
,but only if it's present.